Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Suppress needlessly alarming messages #628

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dabrahams
Copy link

@dabrahams dabrahams commented Apr 3, 2024

These packages are not marked REQUIRED and when this project is used as a dependency of another CMake project they don't need to be findable when this CMakeLists.txt is read. They may in fact be found later in the configuration process, so the messages when they actually are needed, so the messages

-- Could NOT find dispatch (missing: dispatch_DIR)
-- Could NOT find Foundation (missing: Foundation_DIR)
-- Could NOT find XCTest (missing: XCTest_DIR)

are needlessly alarming.

Replace this paragraph with a description of your changes and rationale. Provide links to an existing issue or external references/discussions, if appropriate.

Checklist

  • I've added at least one test that validates that my change is working, if appropriate
  • I've followed the code style of the rest of the project
  • I've read the Contribution Guidelines
  • I've updated the documentation if necessary

These packages are not marked `REQUIRED` and when this project is used as a dependency of another CMake project they don't need to be findable when this CMakeLists.txt is read.  They may in fact be found later in the configuration process, so the messages when they actually _are_ needed, so the messages

```
-- Could NOT find dispatch (missing: dispatch_DIR)
-- Could NOT find Foundation (missing: Foundation_DIR)
-- Could NOT find XCTest (missing: XCTest_DIR)
```

are needlessly alarming.
@natecook1000 natecook1000 requested a review from compnerd April 3, 2024 22:55
Copy link
Collaborator

@compnerd compnerd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose that this is fine. The problem is that QUIET is now applied unconditionally which means that when building the toolchain, we would not see in the logs if a mismatched version of these toolchain vended libraries are accidentally picked up. Perhaps it makes sense to conditionalise this on whether the package is being built as a top-level package?

CC: @etcwilde - Evan may have thoughts on the logging as well.

@compnerd
Copy link
Collaborator

compnerd commented Apr 7, 2024

@swift-ci please test

@etcwilde
Copy link
Member

etcwilde commented Apr 8, 2024

Yeah, I'd prefer to keep the logging. If they aren't a WARNING, and aren't an ERROR, they won't prevent you from continuing and are just informational, but if they aren't emitted and you were expecting to find them, having that in the log is definitely preferable to scratching your head wondering why things were getting built weirdly.

@natecook1000
Copy link
Member

Maybe instead of marking these QUIET, we could add an additional message that contextualizes the output?

@dabrahams
Copy link
Author

dabrahams commented Apr 16, 2024

Maybe you could conditionally find_package based on the conditions that actually require them, and under those conditions make them REQUIRED? The current situation:

  • For developers of this package, defers errors to link time with a much harder-to-understand message, after you've wasted a bunch of time proceeding past an unrecoverable condition
  • For consumers of this package, produces confusing messages—in my case, for example XCTest actually is found, only later. So it's not just alarming, it's misleading to see this message.

@natecook1000 natecook1000 added the help wanted Extra attention is needed label May 17, 2024
@compnerd
Copy link
Collaborator

@dabrahams the condition that you need find_package is not a very helpful heuristic. We cannot infer if the user is intending to build against a custom build of any of those libraries or against the toolchain distribution of the libraries. The common case for building this library with CMake is intended to be against a custom build and you should do a find_package.

I can see an argument for eliding the find_package on macOS as we cannot build the dependencies on macOS in the OSS release, but outside of that, I think that the current behaviour is the preferred option.

@dabrahams
Copy link
Author

@compnerd I don't understand what your comment means. I wasn't suggesting any heuristic. I don't think there's any reason it should make a difference which version of those libraries are needed for any particular build. Assuming you want to keep the find_package calls, all I'm suggesting is that they be marked QUIET.

@compnerd
Copy link
Collaborator

The normal usage for the build is with a custom build of the dependencies and therefore the default should be non-QUIET. The special case is Darwin, where these are system provided, so we could exclude the checks there.

@dabrahams
Copy link
Author

dabrahams commented Sep 23, 2024

I still don't understand the "therefore." Does using a custom build of the dependencies require that they have been set up by that point?

Are they really dependencies at all? dispatch only appears in that file. I… guess Foundation is a real dependency. And XCTest is only a dependency if you're building testing, so it find_package for that should only happen if BUILD_TESTING is set.

@compnerd
Copy link
Collaborator

compnerd commented Sep 24, 2024

I still don't understand the "therefore." Does using a custom build of the dependencies require that they have been set up by that point?

Yes, the custom build of dependencies requires that they are setup by that point (except on Darwin where they are system provided).

Are they really dependencies at all? dispatch only appears in that file. I… guess Foundation is a real dependency.

Yes, dispatch is a transitive dependency due to Foundation.

And XCTest is only a dependency if you're building testing, so it find_package for that should only happen if BUILD_TESTING is set.

That is a great suggestion! We should conditionalise the find_package(XCTest) under BUILD_TESTING.

CMakeLists.txt Outdated Show resolved Hide resolved
Co-authored-by: Saleem Abdulrasool <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants